feat: support named arguments for builtins #1529
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1529 +/- ##
==========================================
- Coverage 94.03% 93.82% -0.21%
==========================================
Files 174 173 -1
Lines 58611 54308 -4303
==========================================
- Hits 55112 50953 -4159
+ Misses 3499 3355 -144 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tests/correctness/functions.rs
Outdated
| assert_eq!(main.res, "world\0".as_bytes()); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
TBH, at this stage i would prefer we don't add any more correctness tests and only use lit tests. We know our correctness tests don't take the full system into consideration when building. We should start planning to report coverage on the lit tests in addition to the current tests and slowly move tests to lit where it makes sense.
There was a problem hiding this comment.
Agreed, lit tests are better here. So I'll remove them.
| @@ -0,0 +1,126 @@ | |||
| // RUN: (%COMPILE %s && %RUN) | %CHECK %s | |||
There was a problem hiding this comment.
I would create the exact same test for unnamed, even if they work, just so we have them in lit. And eventually we want one that mixes.
There was a problem hiding this comment.
Yeah makes sense although I'm a bit torn here. It's nice to have them split into seperate tests but it's also nice to have them all together. But to get an overview over what's tested I agree that having distinclty named tests is nicer. So I added a separate one for positional arguments.
| @@ -0,0 +1,23 @@ | |||
| --- | |||
| source: src/codegen/tests/expression_tests.rs | |||
There was a problem hiding this comment.
You have some dangling snapshots here
There was a problem hiding this comment.
nice catch! will remove them
3 participants
This PR introduces the feature to use not only positional but also named arguments for the following builtins:
SELREFADRUPPER_BOUNDLOWER_BOUNDSIZEOFSUBDIVNote
The following builtins don't support named arguments for now:
MUXGTLTNELEEQGEADDMUL